[SSHD-704] Add support for RFC 8731 #177
Conversation
sshd-core/src/main/java/org/apache/sshd/common/kex/MontgomeryCurve.java
Outdated
Show resolved
Hide resolved
| if (key.length < getKeySize()) { | ||
| throw new InvalidKeySpecException("Provided key is too small for " + getAlgorithm()); | ||
| } | ||
| // ideally, we'd just parse the key as a BigInteger and then create a XECPublicKeySpec in Java 11 |
There was a problem hiding this comment.
Note: turns out the XECPublicKeySpec takes a BigInteger parameter for u, but u when in binary is little endian. Thus, if using the spec class, the array must be reversed into BigInteger, then the XDH engine will reverse it again back to little endian for use in the algorithm. Same applies to the EdDSA APIs added in Java 15. Thus, the DER dumps of ASN.1 data seem to be both portable as well as less redundant as the key is already little endian in the DER form.
|
@jvz What's the status of this PR ? We're planning a 2.7.0 release this week, so wondering if I should merge this PR in. |
|
A test would be nice. |
|
Same for this. I'll revisit to clean up sometime in the next week. |
This adds support for curve25519-sha256 (and its libssh version) along with curve448-sha512. These key exchange methods are supported for both Java 11 and Bouncy Castle. Signed-off-by: Matt Sicker <boards@gmail.com>
|
Came back to this. It's not even activated; one needs to add the new DH factories in BaseBuilder first. If one does so, curve25519 gets chosen for KEX (tested against the Github SSH daemon and aginst the Gitlab SSH daemon). KEX fails; the connection is terminated immediately by the server. Something is missing somewhere, or somewhere along the line we don't do what the RFCs say we should do. |
|
It works for curve25519 if both keys (our and theirs) have the most significant bit zero. If our key (encode()) has the most significant bit set, they close the connection. If their key (decode()) has the most significant bit set, we fail to verify their signature. I suspect this is related to RFC 7748, section 5:
(They talk little-endian here.) |
|
@jvz any ideas? |
Got it. This is caused by a subtle difference in the DH key exchange.
That explains it: Apache MINA sshd implements RFC 4253, but not the modifications for ECDH/XDH from RFC 5656.
The difference is pointed out in RFC 8731:
RFC 8731, section 3.1, then explains that the shared secret (32 bytes for x25519) is always interpreted as unsigned. (But according to RFC 5656, it's still written as mpint.) Our code writes it as mpint, which prefixes a zero byte if the high bit is set, so that should be fine. With this fixed, KEX using curve25519 works against both the Github and Gitlab daemons. @jvz, is it OK if I push a rebase + follow-up change onto this PR? |
|
Yes, that sounds like a great analysis and fix idea! I'll be on vacation for the next week, though I can review PRs from my phone. |
tomaswolf
force-pushed
the
curve25519-SSHD-704
branch
from
d500877
to
e9e6077
Compare
|
Hm, that doesn't seem to work in the CI build. At least with Java 14. I'll have to investigate locally. |
Simplify MontgomeryCurve, take advantage of the fact that the key is always the last 32 or 56 bytes of the X.509 DER encoding. Add extensive comments about the "magic" constants. Be more strict about key data with an extra leading zero byte. Enable the new KEX algorithms in BaseBuilder. At that point, curve25519 KEX worked only if both the client's and the server's ephemeral public keys byte arrays did not have the most significant bit in the first byte set. This uncovered a bug in DHGClient and DHGServer: ECDH and XDH KEX encode Q_C and Q_S as plain byte arrays, not as multi-precision integers. I.e., they _do not_ prefix a zero byte if the high bit in the first byte of the value is set. Fix this by introducing AbstractDH.putE(Buffer) and putF(Buffer) methods, so that individual DH implementations can override as appropriate. Override them for ECDH and XDH to write byte arrays, not mpints. * RFC 4253, section 8:[1] encodes e and f as mpint. * RFC 5656, section 4:[2] Q_C and Q_S, which take the place of e and f, are written as "strings" (byte arrays). * RFC 8731, section 3:[3] Public ephemeral keys are written as strings. With these changes, KEX using curve25519 works, and KEX using one of the other already existing algorithms also still works. [1] https://tools.ietf.org/html/rfc4253#page-22 [2] https://tools.ietf.org/html/rfc5656#page-9 [3] https://tools.ietf.org/html/rfc8731#section-3
tomaswolf
force-pushed
the
curve25519-SSHD-704
branch
from
e9e6077
to
84a35b0
Compare
|
CI builds finally passed; so the failures on Windows in the two earlier CI runs were just unstable tests. Couldn't reproduce them locally either. BTW: this problem with mpint vs. bit strings wasn't noticed earlier because ECDH keys always start with 0x04 (uncompressed) or 0x03 or 0x02 (compressed). But x25519 or x448 keys may have the high bit in the first byte set. This is ready for review. |
|
Thanks for taking a look. Yes, I think the "extra zero byte" handling could be removed now altogether. I decided to leave it in just in case. It doesn't hurt, and it's a minor and harmless deviation from the RFCs (which say to disconnect if you don't get exactly 32 or 56 bytes). |
This adds support for curve25519-sha256 (and its libssh version) along with curve448-sha512. These key exchange methods are supported for both Java 11 and Bouncy Castle.
https://issues.apache.org/jira/browse/SSHD-704
https://tools.ietf.org/html/rfc8731
https://tools.ietf.org/html/rfc7748 (for the X25519/X448 algorithms)